refactor: make BGP peer and secret management system event-driven #790

aritrbas · 2025-09-11T20:30:23Z

Summary

This PR refactors the BGP peer and secret management system from a tightly-coupled handler-based architecture to a loosely-coupled event-driven system.

Changes by Component

1. PeerWatcher (`watchers/peers_watcher.go`)

Added local peer cache: cachedPeers map[string]calicov3.BGPPeer
Added granular events:
- PeerAdded - single new peer added
- PeerUpdated - single existing peer updated
- PeerDeleted - single existing peer deleted
- PeersChanged - for initial state reconciliation only
Updates SecretWatcher proactively on peer list changes
Achieves O(1) cache lookups instead of O(N) API list calls

2. SecretWatcher (`watchers/secret_watcher.go`)

Removed GetSecret() method (blocking) with events
Removed SweepStale() method (replaced with OnPeerListUpdated)
Emits granular events:
- SecretAdded - new secret available
- SecretChanged - existing secret modified
- SecretDeleted - existing secret removed
Proactive secret watching via OnPeerListUpdated() - starts watches when peers are added
Automatic cleanup of stale secret watches when peers are deleted

3. PeerHandler (`routing/peer_handler.go`)

Added local secret cache: secretCache map[string]map[string]string
Added granular peer event handlers:
- OnPeerAdded(peer) - O(1) processing for single peer
- OnPeerUpdated(old, new) - O(1) diff and update
- OnPeerDeleted(peer) - O(1) removal
Added granular secret event handlers:
- OnSecretAdded(secretData) - cache secret locally
- OnSecretChanged(secretName) - mark affected peers for update
- OnSecretDeleted(secretName) - remove from cache
Added ProcessPeers() for initial reconciliation only

4. FelixServer (`felix/felix_server.go`)

Registered for new event types: PeersChanged, PeerAdded, PeerUpdated, PeerDeleted, SecretAdded, SecretChanged, SecretDeleted
Acts as an event coordinator (no business logic); routes events to appropriate PeerHandler methods

Made the changes on top of the nsk-split-svc branch for now, will rebase on top of master once those commits for single thread agent are merged.

sknat

Many thanks for taking a stab at this !
A couple comments inline, I think there is still room for more untangling of the logic.
Tell me if anything sounds unclear

sknat · 2025-10-17T13:48:18Z

calico-vpp-agent/watchers/peers_watcher.go

+	w := &PeerWatcher{
+		log:           log,
+		clientv3:      clientv3,
+		handler:       handler,


Instead of passing a handler here, could we emit events ?
i.e.

felixServer.GetFelixServerEventChan() <- &common.ProcessPeers{peers.Items}

and in the felixServer main loop explicitly calling handlers

func (s *Server) handleFelixServerEvents(msg interface{}) (err error) { ... case *common.ProcessPeers: s.peerHandler.ProcessPeers(evt)

sknat · 2025-10-17T13:49:24Z

calico-vpp-agent/watchers/peers_watcher.go

-	}
-	return nil
+	// Send the peer list to the handler for processing
+	return w.handler.ProcessPeers(peers.Items)


This would then be an event

sknat · 2025-10-17T13:50:02Z

calico-vpp-agent/watchers/peers_watcher.go

+// OnSecretChanged handles secret change events and triggers peer reconciliation
+func (w *PeerWatcher) OnSecretChanged(secretName string) {
+	w.log.Infof("Secret '%s' changed, notifying handler", secretName)
+	w.handler.OnSecretChanged(secretName)


Same thing here, we could make OnSecretChanged be an event,
that way we wouldn't have to keep a reference to the handler here

sknat · 2025-10-17T13:53:50Z

calico-vpp-agent/watchers/peers_watcher.go

+	w.log.Infof("Secret '%s' changed, notifying handler", secretName)
+	w.handler.OnSecretChanged(secretName)
+	// Trigger a resync to apply the new secret
+	err := w.resyncPeers()


I think this shouldn't be needed anymore :
Before it was more convenient to call resyncPeers() as this was doing both scanning, and constituting the local cache of Peers+Secrets.
But now that the reconstitution is done in the handlers, we can do everything there, and maybe call the same code in both cases (peerChanged & secretChanged)

sknat · 2025-10-17T13:56:36Z

calico-vpp-agent/watchers/secret_watcher.go

-func (sw *secretWatcher) OnAdd(obj interface{}, isInInitialList bool) {
+func (sw *SecretWatcher) notifySecretChanged(secretName string) {
+	for _, handler := range sw.handlers {
+		handler.OnSecretChanged(secretName)


If we rely on the fact that the peerWatcher does not need to be informed when the secret changes, and we make a common.OnSecretChanged towards the felix server, we could remove the need to have keep a sw.handler reference.

We could simply

eventChan <- &common.OnSecretChanged{...}

sknat · 2025-10-17T14:01:07Z

calico-vpp-agent/routing/peer_handler.go

+
+	nodeStatesByName map[string]common.LocalNodeSpec
+
+	secretWatcher watchers.SecretGetter


We could probably remove the reference to the secretWatcher by adding a secret store in the peerHandler. We could create events emitted by the secretWatcher

common.SecretAdded{...} common.SecretDelete{...}

that would be received in the felixServer event loop,
call felix.peerHandler.OnSecret{Added,Deleted}(evt)
which would update a local cache map.

Below h.secretWatcher.GetSecret(secretName, secretKey) just becomes a lookup in the map in question

sknat · 2025-10-17T14:06:15Z

calico-vpp-agent/watchers/peers_watcher.go

+		log:           log,
+		clientv3:      clientv3,
+		handler:       handler,
+		secretWatcher: secretWatcher,


I think for simplicity, if we remove the need for handler's registration by introducing events
we could instantiate the secretWatcher directly here
i.e.

secretWatcher: watchers.NewSecretWatcher(k8sclient, log.WithFields(logrus.Fields{"component": "secret-watcher"}))

sknat · 2025-10-17T14:09:00Z

calico-vpp-agent/watchers/peers_watcher.go

+	activeSecrets := make(map[string]struct{})
+	for _, peer := range peers.Items {
+		if peer.Spec.Password != nil && peer.Spec.Password.SecretKeyRef != nil {
+			secretName := peer.Spec.Password.SecretKeyRef.Name
+			activeSecrets[secretName] = struct{}{}
+		}
 	}
-	return true
-}
+	w.secretWatcher.SweepStale(activeSecrets)


Could we make this w.secretWatcher.OnPeerListUpdated(peers.Items) and have the sweep logic in the secret watcher ?

sknat · 2025-10-17T14:11:12Z

calico-vpp-agent/watchers/secret_watcher.go

 }

-func (sw *secretWatcher) GetSecret(name, key string) (string, error) {
+func (sw *SecretWatcher) GetSecret(name, key string) (string, error) {


Following on the comment above, we could directly start watching for secrets when OnPeerListUpdated() is called with a new Peer that has secrets attached.

And emit events when we receive the secret's contents.

sknat · 2025-10-17T14:16:08Z

calico-vpp-agent/watchers/peers_watcher.go

-						w.log.Warn("Unrecognized secret change event received. Ignoring...")
-					}
+				// Re-sync peers when watch events occur
+				err = w.resyncPeers()


Now that we have an isolated watcher, we could implement the watch logic :

we would cache the list of peers locally on the peerWatcher

when update/delete/create happens, we update the list, and send a copy as an event over the eventChan

We only resort to listing again if an error happens

Further simplification would be to send separated events for ADD/DEL/UPD over the eventchan, but this will require changing the handler's logic which might be beyond the scope of this PR

Signed-off-by: Aritra Basu <[email protected]>

aritrbas requested review from florincoras, hedibouattour, onong and sknat September 11, 2025 20:30

aritrbas self-assigned this Sep 11, 2025

aritrbas force-pushed the abasu-peers-watcher-rem-pubsub branch from 6fee584 to 2c22c24 Compare October 1, 2025 04:18

aritrbas mentioned this pull request Oct 2, 2025

refactor: decouple routingServer into BGPWatcher, BGPHandler and routingHandler #800

Open

aritrbas force-pushed the abasu-peers-watcher-rem-pubsub branch 2 times, most recently from ffb8c29 to eb5adeb Compare October 10, 2025 00:20

sknat reviewed Oct 17, 2025

View reviewed changes

refactor: make BGP peer and secret management system event-driven

d64a12d

Signed-off-by: Aritra Basu <[email protected]>

aritrbas force-pushed the abasu-peers-watcher-rem-pubsub branch from eb5adeb to d64a12d Compare October 23, 2025 22:34

aritrbas changed the title ~~Remove peers_watcher dependency on pubsub~~ refactor: make BGP peer and secret management system event-driven Oct 23, 2025


		nodeStatesByName map[string]common.LocalNodeSpec

		secretWatcher watchers.SecretGetter

refactor: make BGP peer and secret management system event-driven #790

Are you sure you want to change the base?

refactor: make BGP peer and secret management system event-driven #790

Uh oh!

Conversation

aritrbas commented Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes by Component

1. PeerWatcher (watchers/peers_watcher.go)

2. SecretWatcher (watchers/secret_watcher.go)

3. PeerHandler (routing/peer_handler.go)

4. FelixServer (felix/felix_server.go)

Uh oh!

sknat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aritrbas commented Sep 11, 2025 •

edited

Loading

1. PeerWatcher (`watchers/peers_watcher.go`)

2. SecretWatcher (`watchers/secret_watcher.go`)

3. PeerHandler (`routing/peer_handler.go`)

4. FelixServer (`felix/felix_server.go`)